Codecov Report

Attention: Patch coverage is 74.19355% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.37%. Comparing base (86a8ba9) to head (c426114).
Report is 41 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1667      +/-   ##
==========================================
+ Coverage   75.07%   76.37%   +1.30%     
==========================================
  Files         234      241       +7     
  Lines       12156    18469    +6313     
==========================================
+ Hits         9126    14106    +4980     
- Misses       2582     3901    +1319     
- Partials      448      462      +14     

@BuidlWithShivam,

Thank you so much for this amazing PR. It’s great to see how smart you are in understanding the logic and fixing this issue. Good job!
By thee way, I feel we can improve it in some ways.

First of all, it’s great to see that you can detect the type of PrivateKey by their prefixes. This approach is better than checking for errors in the public key. We can do the same for the public key as well, like this:

	maybeBLSPublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "public1p")
	}

	maybeEd25519PublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "public1r")
	}
	switch {
	case maybeBLSPublicKey(req.PublicKey):
		/// ...

	case maybeEd25519PublicKey(req.PublicKey):
		/// ...

	default:
		return nil, status.Error(codes.InvalidArgument, "invalid public key")
	}

Now, we can make another improvement here. PublicKeys are interfaces. We can simplify the above code by creating a function that takes a string and returns a PublicKey as an interface.

We can define a function like this:

func (*utilServer) publicKeyFromString(pubStr string) (crypto.PublicKey, error) {
	maybeBLSPublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "secret1p")
	}

	maybeEd25519PublicKey := func(str string) bool {
		return strings.Contains(strings.ToLower(str), "secret1e")
	}

	switch {
	case maybeBLSPublicKey(pubStr):
		blsPub, err := bls.PublicKeyFromString(pubStr)
		if err != nil {
			return nil, status.Error(codes.InvalidArgument, "invalid private key")
		}
		return blsPub, nil

	case maybeEd25519PublicKey(pubStr):
		ed25519Pub, err := ed25519.PublicKeyFromString(pubStr)
		if err != nil {
			return nil, status.Error(codes.InvalidArgument, "invalid private key")
		}
		return ed25519Pub, nil

	default:
		return nil, status.Error(codes.InvalidArgument, "invalid private key")
	}
}

Finally we can make the same for PrivateKey

@b00f Sure. Thanks for the review. I was not aware that the public key also has the same structure. Tried to find across the code but couldn't. Thanks for the suggestion on the interface as well. I just saw two different structs and missed the interface. I will implement those.

@b00f Made the changes. Can you please check.

@BuidlWithShivam

You can use make devtools followed by make check to ensure that your changes have no formatting or linting issues. For more details, refer to the Contributing Guide.

@BuidlWithShivam
I decided to use error checking instead of prefix checking. Using error checking makes this much simpler, reducing the code by almost 20 lines.

Good job @BuidlWithShivam

@BuidlWithShivam Do you like to join Pactus DEV team?

@BuidlWithShivam Do you like to join Pactus DEV team?

@b00f I am looking for opportunities in Blockchain. Sure we can have a chat about it.

@BuidlWithShivam Please join our Discord and let's have chat there: https://discord.gg/pactus

@BuidlWithShivam Please join our Discord and let's have chat there: https://discord.gg/pactus

@b00f Joined.